Add decrease interface for aggregation #9737

xzhangxian1008 · 2024-12-20T03:13:23Z

What problem does this PR solve?

Issue Number: ref #7376

Problem Summary:

What is changed and how it works?

In order to support aggregation in window function, we need to add some interfaces for aggregation. In this pr, we add `decrease` interface for aggregation.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

xzhangxian1008 · 2025-01-06T01:43:55Z

/retest

dbms/src/AggregateFunctions/AggregateFunctionCount.h

guo-shaoge · 2025-01-06T03:34:16Z

dbms/src/AggregateFunctions/IAggregateFunction.h

+
+    void decrease(AggregateDataPtr __restrict, const IColumn **, const size_t, Arena *) const override
+    {
+        throw Exception("Not implemented yet");


add method name and agg func name in error message?

add method name and agg func name in error message?

done

guo-shaoge · 2025-01-06T07:21:36Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxAny.h

@@ -594,6 +600,10 @@ struct AggregateFunctionMinData : Data
    }
    bool changeIfBetter(const Self & to, Arena * arena) { return this->changeIfLess(to, arena); }

+    void prepareWindow() { throw Exception("Not implemented yet"); }


Looks like SingleValueDataGeneric derived from CommonImpl, so prepareWindow is not needed here?

Looks like SingleValueDataGeneric derived from CommonImpl, so prepareWindow is not needed here?

done

guo-shaoge · 2025-01-06T07:21:53Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxAny.h

@@ -602,10 +612,13 @@ struct AggregateFunctionMaxData : Data
 {
    using Self = AggregateFunctionMaxData<Data>;

+    void insertResultInto(IColumn & to) const { Data::insertResultInto(to); }


ditto

done

guo-shaoge · 2025-01-06T07:22:30Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxAny.h

@@ -594,6 +600,10 @@ struct AggregateFunctionMinData : Data
    }
    bool changeIfBetter(const Self & to, Arena * arena) { return this->changeIfLess(to, arena); }

+    void prepareWindow() { throw Exception("Not implemented yet"); }
+
+    void insertResultInto(IColumn & to) const { Data::insertResultInto(to); }


this can be removed?

this can be removed?

done

guo-shaoge · 2025-01-06T07:31:14Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+
+    bool changeIfLess(const Self & to, Arena * arena)
+    {
+        if (saved_values != nullptr)


Is this necessary ? you mean prepareWindow() is not always called?
but reset() didn't check if saved_values is nullptr

Is this necessary ? you mean prepareWindow() is not always called? but reset() didn't check if saved_values is nullptr

Yes, in some circumstance, we will never call decrease function.

xzhangxian1008 · 2025-01-07T06:14:48Z

/cc @guo-shaoge @windtalker

xzhangxian1008 · 2025-01-08T05:34:07Z

/cc @guo-shaoge

windtalker · 2025-01-07T08:19:11Z

dbms/src/AggregateFunctions/AggregateFunctionFactory.cpp

+    int recursion_level) const
+{
+    AggregateFunctionCombinatorPtr combinator
+        = AggregateFunctionCombinatorFactory::instance().tryFindSuffix("NullForWindow");


even if the argument is not null, it still need this combinator?

even if the argument is not null, it still need this combinator?

yes

windtalker · 2025-01-07T08:20:42Z

dbms/src/AggregateFunctions/AggregateFunctionForEach.h

@@ -75,7 +75,10 @@ class AggregateFunctionForEach final
        size_t old_size = state.dynamic_array_size;
        if (old_size < new_size)
        {
-            state.array_of_aggregate_datas = arena.realloc(
+            if unlikely (arena == nullptr)
+                throw Exception("Get nullptr in ensureAggregateData");


Suggested change

throw Exception("Get nullptr in ensureAggregateData");

throw Exception("Get null arena ptr in ensureAggregateData");

windtalker · 2025-01-07T08:24:09Z

dbms/src/AggregateFunctions/AggregateFunctionForEach.h

+        }
+    }
+
+    void prepareWindow(AggregateDataPtr __restrict place) const override


I suggest that for the agg function that is not supported by window operator, do not implement these window function specific interface, since it will never be used and can not be well tested.

I suggest that for the agg function that is not supported by window operator, do not implement these window function specific interface, since it will never be used and can not be well tested.

done

windtalker · 2025-01-07T08:32:07Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+    using Self = SingleValueDataFixedForWindow<T>;
+    using ColumnType = std::conditional_t<IsDecimal<T>, ColumnDecimal<T>, ColumnVector<T>>;
+
+    mutable std::deque<T> * saved_values;


why need use raw pointer here

why need use raw pointer here

we now drop pointer now.

windtalker · 2025-01-08T06:30:46Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+    void changeIfLess(const IColumn & column, size_t row_num, Arena * arena)
+    {
+        auto to_value = static_cast<const ColumnType &>(column).getData()[row_num];
+        if (saved_values != nullptr)


why can't we ensure that saved_values must be not null? If the call fallback to SingleValueDataFixed<T>::changeIfLess(column, row_num, arena); it seems meaningless.

why can't we ensure that saved_values must be not null? If the call fallback to SingleValueDataFixed<T>::changeIfLess(column, row_num, arena); it seems meaningless.

The check for saved_values has been deleted.

windtalker · 2025-01-08T06:49:44Z

dbms/src/AggregateFunctions/AggregateFunctionNull.h

+
+        if (result_is_nullable && need_counter)
+        {
+            auto tmp = prefix_size;


if prefix_size >= sizeof(Int64) there is no need to use more prefix size?

if prefix_size >= sizeof(Int64) there is no need to use more prefix size?

no, when prefix_size == sizeof(Int64) the null flag always needs 1 byte, in this case we need more prefix size, when prefix_size > sizeof(Int64), in order to ensure the alignment, we may still need to add prefix size to ensure the alignment.

windtalker · 2025-01-08T06:56:27Z

dbms/src/AggregateFunctions/AggregateFunctionNull.h

+        if constexpr (is_add)
+        {
+            this->addCounter(place);
+            this->setFlag(place);


why still need the flag if we already has counter?

why still need the flag if we already has counter?

Because we inherit AggregateFunctionNullBase and AggregateFunctionNullBase needs this flag.

xzhangxian1008 · 2025-01-09T07:57:07Z

/cc @windtalker @guo-shaoge

xzhangxian1008 · 2025-01-09T08:52:08Z

/unhold

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

guo-shaoge · 2025-01-10T02:22:34Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

+            if constexpr (is_min)
+                iter = saved_values.begin();
+            else
+                iter = --(saved_values.end());


Suggested change

iter = --(saved_values.end());

auto iter = std::prev(saved_values.end());

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h

guo-shaoge · 2025-01-10T02:29:33Z

dbms/src/AggregateFunctions/AggregateFunctionMinMaxAny.h

 namespace DB
 {
 /** Aggregate functions that store one of passed values.
  * For example: min, max, any, anyLast.
  */

+struct CommonImpl
+{
+    static void decrease(const IColumn &, size_t) { throw Exception(" decrease is not implemented yet"); }


why extra space before error msg?

why extra space before error msg?

done

dbms/src/AggregateFunctions/AggregateFunctionForEach.h

guo-shaoge · 2025-01-10T02:36:03Z

other lgtm

ti-chi-bot · 2025-01-10T05:51:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guo-shaoge

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [guo-shaoge]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2025-01-10T05:51:18Z

[LGTM Timeline notifier]

Timeline:

2025-01-10 05:51:18.42432135 +0000 UTC m=+505621.713153055: ☑️ agreed by guo-shaoge.

ti-chi-bot · 2025-01-10T06:18:15Z

@xzhangxian1008: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-integration-test	`c7817b5`	link	true	`/test pull-integration-test`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

xzhangxian1008 added 19 commits November 26, 2024 16:58

init

3f80eed

add gtest

171fd46

add ft

943c578

refine test framework

e0b05a2

fix compilation phase

75d2f12

init

7022607

codes done, need tests

b011bfa

save

8628a2e

add tests

db4e399

add sum tests

a9360da

refine tests

e3c31bd

format

e3b9756

fix bugs

a159857

refine test

0d4401d

fix tests

9ff2bd3

add test for string type

a9aa879

add test for SingleValueDataGeneric type

bf652bf

tweaking

61eaef0

Merge branch 'master' into wagg

6017780

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 20, 2024

xzhangxian1008 added 9 commits December 20, 2024 11:41

remove something

2358efb

revoke

3227b50

add AlignedBuffer

b2772ea

remove something

37f4cc5

tweaking

dd52c9f

remove useless change

6379b0f

fix ci

9e41b3a

fix ci

e6adcbf

fix ut

b84c911

guo-shaoge reviewed Jan 6, 2025

View reviewed changes

ti-chi-bot bot requested a review from guo-shaoge January 7, 2025 06:14

xzhangxian1008 added 2 commits January 8, 2025 12:31

address comments

4a550c7

address comments

2c7e362

windtalker reviewed Jan 8, 2025

View reviewed changes

xzhangxian1008 added 2 commits January 9, 2025 13:17

replace queue with tree

917892b

remove useless codes

8592277

ti-chi-bot bot requested a review from windtalker January 9, 2025 07:57

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2025

guo-shaoge reviewed Jan 10, 2025

View reviewed changes

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h Outdated Show resolved Hide resolved

guo-shaoge reviewed Jan 10, 2025

View reviewed changes

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h Show resolved Hide resolved

guo-shaoge reviewed Jan 10, 2025

View reviewed changes

dbms/src/AggregateFunctions/AggregateFunctionMinMaxWindow.h Outdated Show resolved Hide resolved

guo-shaoge reviewed Jan 10, 2025

View reviewed changes

dbms/src/AggregateFunctions/AggregateFunctionForEach.h Outdated Show resolved Hide resolved

address comment

0d6b0d1

xzhangxian1008 requested a review from guo-shaoge January 10, 2025 05:48

guo-shaoge approved these changes Jan 10, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jan 10, 2025

ti-chi-bot bot added the approved label Jan 10, 2025

tweaking

c7817b5

	throw Exception("Get nullptr in ensureAggregateData");
	throw Exception("Get null arena ptr in ensureAggregateData");

	iter = --(saved_values.end());
	auto iter = std::prev(saved_values.end());

Add decrease interface for aggregation #9737

Are you sure you want to change the base?

Add decrease interface for aggregation #9737

Conversation

xzhangxian1008 commented Dec 20, 2024 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

xzhangxian1008 commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhangxian1008 commented Jan 7, 2025

xzhangxian1008 commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xzhangxian1008 commented Jan 9, 2025

xzhangxian1008 commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guo-shaoge commented Jan 10, 2025

ti-chi-bot bot commented Jan 10, 2025

ti-chi-bot bot commented Jan 10, 2025

[LGTM Timeline notifier]

ti-chi-bot bot commented Jan 10, 2025

xzhangxian1008 commented Dec 20, 2024 •

edited

Loading